Fix for #18 - support running with --no-scripts flag #33

Ocramius · 2016-07-22T13:15:29Z

Provides fix for #18

…resent in its place

… `composer.lock` assumed location

…age is installed with `--no-scripts`

…ow in place

nikolaposa · 2016-07-22T14:03:49Z

src/PackageVersions/FallbackVersions.php

+    private static function getComposerLockPath() : string
+    {
+        // bold assumption, but there's not here to fix everyone's problems.
+        $checkedPaths = [__DIR__ . '/../../../../../composer.lock', __DIR__ . '/../../composer.lock'];


@Ocramius Perhaps some $path = dirname(__DIR__, $level++) . '/composer.lock'-based check as a better chance alternative? Of course with some reasonable threshold for $level increment.

@nikolaposa I don't trust intermediate levels

nikolaposa · 2016-07-22T14:11:44Z

And what about some totally different approach of triggering Installer at certain event (autoloading?), or at least lazy loading (replacing) this dynamic Versions class after getVersion() is invoked for the first time?

Ocramius · 2016-07-22T14:23:34Z

And what about some totally different approach of triggering Installer at certain event (autoloading?)

Assuming read-only FS for vendor after deployment. Otherwise, we introduce possible security issues. I simply want to allow usage for people running with --no-scripts, but I don't really care if they have a performance impact then: their problem for not optimizing at install-time.

Ocramius · 2016-07-22T14:38:32Z

Alright, this goes in and I will make a release :-)

stof · 2016-07-22T14:57:53Z

There is a big drawback in this PR: it means that a normal install will leave the package in a dirty state, which will cause issues on next updates for people installing from source

Ocramius · 2016-07-22T15:00:56Z

@stof yeh, no real way around it, for now :-\

stof · 2016-07-22T15:06:05Z

test/PackageVersionsTest/FallbackVersionsTest.php

+            );
+        }
+
+        rename(__DIR__ . '/../../composer.lock.backup', __DIR__ . '/../../composer.lock');


this will not rename things properly when there is a test failure

this should be done in a try/finally

stof · 2016-07-22T15:09:38Z

src/PackageVersions/FallbackVersions.php

+
+    private static function getVersions(string $composerLockFile) : \Generator
+    {
+        $lockData = json_decode(file_get_contents($composerLockFile), true);


you should actually check installed packages, not locked ones. packages-dev will always be there in Composer 1.0+, even dev deps are not installed

Is there an array key with the installed packages? I currently merge packages-dev + packages, and I know that dev packages may not have been installed.

installed packages are not in the lock file. they are in vendor/composer/installed.json (and btw, the path between your package and this folder will always be the same even when using custom vendor directories, as they are both inside it)

Right, but what interests us is just the version of a package. For example,
I may have generated(in a CI/CD environment running with dev dependencies)
an asset management system that generates files. Even though the package
doesn't make it to production, the version of the asset manager used to
generate the files is relevant for signature verification purposes. That is
the primary purpose of this library, at least in how I envisioned it when I
built it for my own needs.

On 22 Jul 2016 17:17, "Christophe Coevoet" notifications@github.com wrote:

In src/PackageVersions/FallbackVersions.php
#33 (comment)
:

if (file_exists($path)) {

return $path;

}

}

throw new \UnexpectedValueException(sprintf(

'PackageVersions could not locate your `composer.lock` location. This is assumed to be in %s. '

. 'If you customized your composer vendor directory and ran composer installation with --no-scripts, '

. 'then you are on your own, and we can\'t really help you. Fix your shit and cut the tooling some slack.',

json_encode($checkedPaths)

));

}

private static function getVersions(string $composerLockFile) : \Generator

{

$lockData = json_decode(file_get_contents($composerLockFile), true);

installed packages are not in the lock file. they are in
vendor/composer/installed.json (and btw, the path between your package
and this folder will always be the same even when using custom vendor
directories, as they are both inside it)

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/Ocramius/PackageVersions/pull/33/files/0a1a91ea4d08baa225724845431bee542773fd1a#r71895649,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakC16NEtnqktbklFgVXJ032yDe_Jcks5qYN8SgaJpZM4JSv1j
.

Oh, so it is OK if the package is not there anymore ? This should be clearly documented then

Yes, if the package is not installed (because of non-dev install) then the
version will still be retrievable

On 22 Jul 2016 18:12, "Christophe Coevoet" notifications@github.com wrote:

In src/PackageVersions/FallbackVersions.php
#33 (comment)
:

if (file_exists($path)) {

return $path;

}

}

throw new \UnexpectedValueException(sprintf(

'PackageVersions could not locate your `composer.lock` location. This is assumed to be in %s. '

. 'If you customized your composer vendor directory and ran composer installation with --no-scripts, '

. 'then you are on your own, and we can\'t really help you. Fix your shit and cut the tooling some slack.',

json_encode($checkedPaths)

));

}

private static function getVersions(string $composerLockFile) : \Generator

{

$lockData = json_decode(file_get_contents($composerLockFile), true);

Oh, so it is OK if the package is not there anymore ? This should be
clearly documented then

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/Ocramius/PackageVersions/pull/33/files/0a1a91ea4d08baa225724845431bee542773fd1a#r71904545,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakNm1YtoJ-XgYCngwzUo3k-RhKxvcks5qYOv7gaJpZM4JSv1j
.

The readme should explain it then

Ocramius added 6 commits July 22, 2016 14:54

#18 the Versions class is not to be gitignored, as a stub will be p…

361a532

…resent in its place

#18 tests for a FallbackVersions provider that can work with just a…

ecf8760

… `composer.lock` assumed location

#18 implemented FallbackVersions as per specification

20783c9

#18 stubbed out Versions class, so that it works even when the pack…

c70fd71

…age is installed with `--no-scripts`

#18 gitignoring PackageVersions\Versions again, since the stub is n…

9599960

…ow in place

#18 testing with --no-scripts

0a1a91e

Ocramius added the bug label Jul 22, 2016

Ocramius added this to the 1.1.0 milestone Jul 22, 2016

Ocramius self-assigned this Jul 22, 2016

Ocramius mentioned this pull request Jul 22, 2016

Fails when using composer install --no-scripts #18

Closed

nikolaposa reviewed Jul 22, 2016
View reviewed changes

Ocramius merged commit fe7e599 into master Jul 22, 2016

Ocramius deleted the fix/#18-support-running-with--no-scripts-flag branch July 22, 2016 14:38

stof mentioned this pull request Jul 22, 2016

Fix incompatibility with --classmap-authoritative #27

Closed

stof reviewed Jul 22, 2016
View reviewed changes

Ocramius added a commit that referenced this pull request Jul 22, 2016

Removing files directive: not needed anymore, as per #33

86f2636

stof reviewed Jul 22, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #18 - support running with --no-scripts flag #33

Fix for #18 - support running with --no-scripts flag #33

Ocramius commented Jul 22, 2016

nikolaposa Jul 22, 2016

Ocramius Jul 22, 2016

nikolaposa commented Jul 22, 2016

Ocramius commented Jul 22, 2016

Ocramius commented Jul 22, 2016

stof commented Jul 22, 2016

Ocramius commented Jul 22, 2016

stof Jul 22, 2016

stof Jul 22, 2016

Ocramius Jul 22, 2016

stof Jul 22, 2016

Ocramius Jul 22, 2016

stof Jul 22, 2016

Ocramius Jul 22, 2016

stof Jul 22, 2016

Ocramius Jul 22, 2016

stof Jul 22, 2016

Fix for #18 - support running with --no-scripts flag #33

Fix for #18 - support running with --no-scripts flag #33

Conversation

Ocramius commented Jul 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikolaposa commented Jul 22, 2016

Ocramius commented Jul 22, 2016

Ocramius commented Jul 22, 2016

stof commented Jul 22, 2016

Ocramius commented Jul 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment